[vite-plugin] Don't advertise a dev session's runtime until it is final - #15024
[vite-plugin] Don't advertise a dev session's runtime until it is final#15024petebacondarwin wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: c1da2d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
|
The config includes an One thing worth verifying: The implementation and tests are correct and well-reasoned. Let me do a final check on whether there could be an issue with Let me verify the The change is well-designed, correct, tested, and documented. Let me confirm there are no lint concerns like floating promises in the new code (the fixture sampler IIFE is awaited via The code is clean. No actionable logic bugs, security issues, backward-compat violations, or incorrect API behavior. The implementation correctly:
LGTM |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
d699b3f to
cb72c40
Compare
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
The plugin starts workerd twice: once to discover each Worker's exports by running it, then again with a config built from what it found. The first runtime was published to the dev registry and then torn down, leaving peers holding a debug port that no longer existed. On Windows a peer with a tail_consumers edge to it aborts its own workerd with std::terminate. Miniflare gains unsafeDeferDevRegistryRegistration to hold back self-advertisement, and unsafeRegisterInDevRegistry() to release it once the runtime is final. Reading the registry is unaffected, so a starting session still resolves Workers from sessions already running.
cb72c40 to
c1da2d7
Compare
|
Moving this back to draft while I re-check the premise. Windows validation (fixture temporarily un-skipped, 4 rounds, arms toggled per round) says two things: The change does what it claims. The new invariant test — only one debug port is ever advertised for a Worker during startup — passes in both deferred rounds and fails in both control rounds, on Windows. So the double-advertisement is real and this removes it. But the causal story behind it is wrong. I had assumed a peer is aborted because it resolves a debug port that is about to disappear. In the instrumented logs the Consistent with that, the flake is not fixed: 3 aborts / 1 failure in one round set, 4 aborts / 3 failures in another. There is also a possible regression I have not been able to rule out: with the deferral on, none of 4 aborts reached the end of the runtime reassembly, whereas with it off 2 of 2 recovered and every pre-fix log recovered. That evidence is confounded, because the harness only dumps a session's output when its test fails, so I may be looking at truncated buffers rather than a hang. I don't want this merged until that is settled either way. Keeping #15018's Windows skip in place. Tracking the underlying abort in cloudflare/workerd#6913. |
|
Correction, and I think the actual mechanism. Retracting the suspected regression. I said the deferral might be breaking crash recovery, because none of 4 aborts reached the end of the runtime reassembly. That was wrong, and it was the measurement artefact I flagged. With the harness changed to dump every session rather than only failed ones, 23 aborts break down as 18 that complete recovery and 5 that don't — and every crashed session's log was captured within 0.02–0.4s of its last line, so the 5 simply had no time to finish. Two of those 3 stalls were in the control arm anyway. Crash recovery is fine; this change does not affect it. The actual trigger. Instrumenting the registry push shows the abort consistently follows a push whose payload has shrunk. Across 10 distinct aborts the preceding line is a push of The source of that shrinking is unconditional: So this PR cannot fix the flake, by construction. It changes when a session first advertises itself; it does nothing about entries being deleted and restored on every later config update, or at teardown. The double-advertisement it removes is real, and the invariant test does discriminate on Windows, but it is a tidiness fix rather than the flake fix I originally claimed. The promising fix looks like not deleting entries when the registry path hasn't changed — though it can't be a plain early-return, since Workers genuinely removed from the config still need their entries dropped, so it needs a diff rather than delete-then-recreate. Separately filed #15035 for the shared internal Worker names ( |
|
Closing: the premise behind this doesn't hold. I opened it believing a peer was being aborted because it resolved a debug port that was about to disappear. Instrumented Windows runs showed the abort lands immediately after a session finishes registering, not while any peer holds a stale address, and four validation rounds with the change in place still aborted and still failed the target test. So this was never going to fix the flake, and I'd rather not carry a change whose stated rationale is wrong. The double-advertisement it removed is real, and the invariant test did discriminate on Windows, so if anyone wants that tightened up later it's worth revisiting on its own terms — but as a deliberate tidy-up rather than a crash fix. What came out of the investigation instead:
#15018's Windows skip stays in place. |
2 participants
A
vite devsession publishes aworkerddebug port to the dev registry and then immediately replaces the runtime behind it, so another dev session can be left holding an address that no longer exists.What happens
The Vite plugin brings
workerdup twice while starting: once to discover each Worker's exports by running it, then again with a config assembled from what it found (dev.ts, thehasChangedbranch). The first runtime is advertised in the dev registry and then torn down. A peer that resolves the Worker during that window gets a debug port that is about to disappear — and on Windows, a peer with atail_consumersedge to it can abort its own runtime with*** std::terminate() called with no exception(reported upstream as cloudflare/workerd#6913).Miniflare already unregisters before restarting, but peers only learn by watching the registry directory (polling on Windows) and then pushing the update into their own proxy Worker. That propagation loses the race against a ~150ms restart, so this closes the window rather than trying to shrink it.
The change
miniflaregainsunsafeDeferDevRegistryRegistration, which holds back advertising this instance's Workers, andunsafeRegisterInDevRegistry()to release the hold once the runtime is final. The hold is re-armed by eachsetOptions()that asks to defer, so a dev server restart gets the same protection. Reading the registry is untouched, so a starting session still resolves Workers from sessions already running.@cloudflare/vite-plugindefers registration and releases it at the end ofconfigureServer, unconditionally — so it applies whether or not the export types turned out to differ.The fixture test asserts the invariant directly: exactly one debug port is ever advertised for a Worker during startup. Without the fix it observes two.
Scope — this does not fix the Windows flake
fixtures/dev-registryis currently skipped on Windows (#15018). I temporarily re-enabled it and ran thevite dev <-> vite devsuite four times on a Windows runner to check whether this change is enough to lift that skip. It is not. One of four rounds ofsupports exported handler fetch over service bindingstill timed out at 50s, and threestd::terminateaborts still occurred.So this is a real bug with a targeted fix and a regression test, but the skip in #15018 should stay until the remaining trigger is found. I'm continuing to investigate that separately; the leading suspect is that
#registerWorkersadvertises every named Worker, and the plugin names its internals with global constants (__asset-worker__,__router-worker__,__vite_proxy_worker__), so concurrent vite sessions overwrite and delete each other's entries in the shared registry.unsafe*internal APIs for consumers that stage their runtime startup, and the user-visible effect is a bug fix.Four Miniflare tests plus one fixture test, all confirmed to fail without the change.
A picture of a cute animal (not mandatory, but encouraged)
Note
This is a contribution from an AI agent: OpenCode, claude-opus-5.